Skip to content

Don't use callbacks when performing sync IO with async handles#126845

Merged
adamsitnik merged 20 commits into
mainfrom
copilot/remove-callbacks-sync-io
Jun 12, 2026
Merged

Don't use callbacks when performing sync IO with async handles#126845
adamsitnik merged 20 commits into
mainfrom
copilot/remove-callbacks-sync-io

Conversation

Copilot AI commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

Description

When performing synchronous I/O on async file handles (FILE_FLAG_OVERLAPPED), RandomAccess.Windows.cs previously used ThreadPoolBoundHandle.UnsafeAllocateNativeOverlapped with an IOCompletionCallback and a custom CallbackResetEvent class that tracked a reference count to handle the race between the IOCP callback and the caller's GetOverlappedResult.

This callback path is unnecessary. Setting the low-order bit of hEvent in the OVERLAPPED structure prevents the I/O completion from being queued to the completion port, eliminating the callback.

This reduces complexity and allocations, preserves managed wait behavior, fixes correctness issues in both error handling and exceptional wait paths, and hardens handle lifetime safety in the sync-over-async path.

Copilot AI requested review from Copilot and removed request for Copilot April 13, 2026 17:28
Copilot AI linked an issue Apr 13, 2026 that may be closed by this pull request
…vent for sync IO with async handles

Instead of using ThreadPoolBoundHandle callbacks and reference counting when
performing synchronous IO with async file handles, allocate the NativeOverlapped
directly via NativeMemory and set the low-order bit of hEvent to suppress I/O
completion port notifications. This eliminates the CallbackResetEvent class,
the s_callback field, and the AllocateCallback/GetNativeOverlappedForAsyncHandle
methods, reducing allocations and complexity.

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/d7a61400-c63f-401a-9503-0313459af6f7

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot April 13, 2026 17:58
Copilot AI changed the title [WIP] Remove callbacks for sync IO operations with async handles Don't use callbacks when performing sync IO with async handles Apr 13, 2026
Copilot AI requested a review from adamsitnik April 13, 2026 18:04
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

@adamsitnik adamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address my feedback (nits)

Comment thread src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs Outdated
…appedWithEventHandle

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/bcdd8838-718e-4561-abf4-90c9c2deb905

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot April 14, 2026 05:47
Copilot AI requested a review from adamsitnik April 14, 2026 05:48
@adamsitnik adamsitnik marked this pull request as ready for review April 14, 2026 05:53
Copilot AI review requested due to automatic review settings April 14, 2026 05:53
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

Copilot AI commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

@copilot please address the feedback

Implemented in 569ccce. The remaining feedback items are addressed: stack NativeOverlapped plus DangerousAddRef/DangerousRelease guarding in sync-over-async paths.

Copilot AI requested a review from adamsitnik June 9, 2026 12:54
Comment thread src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs Outdated
Copilot AI review requested due to automatic review settings June 9, 2026 15:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

Comment thread src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@adamsitnik

adamsitnik commented Jun 12, 2026

Copy link
Copy Markdown
Member

I can see that most of the Windows CI legs are timeing out after giving up on AllocZeroed

As of now, I can't reproduce it locally (I can run all tests in less than 12 minutes)

Details
.\build.cmd -c release -subset clr+libs+libs.tests
.\build.cmd -c release -subset libs.tests -test -testnobuild
D:\projects\runtime_copilot\eng\testing\tests.targets(210,5): error : One or more tests failed while running tests from 'System.Net.Security.Tests'. Please check D:\projects\runtime_copilot\artifacts\bin\System.Net.Security.Tests\Release\net11.0-windows\testResults.xml for details! [D:\projects\runtime_copilot\src\libraries\System.Net.Security\tests\FunctionalTests\System.Net.Security.Tests.csproj::TargetFramework=net11.0-windows]
D:\projects\runtime_copilot\eng\testing\tests.targets(210,5): error : One or more tests failed while running tests from 'System.Runtime.Intrinsics.Tests'. Please check D:\projects\runtime_copilot\artifacts\bin\System.Runtime.Intrinsics.Tests\Release\net11.0-windows\testResults.xml for details! [D:\projects\runtime_copilot\src\libraries\System.Runtime.Intrinsics\tests\System.Runtime.Intrinsics.Tests.csproj::TargetFramework=net11.0-windows]
D:\projects\runtime_copilot\eng\testing\tests.targets(210,5): error : One or more tests failed while running tests from 'System.IO.FileSystem.Tests'. Please check D:\projects\runtime_copilot\artifacts\bin\System.IO.FileSystem.Tests\Release\net11.0-windows\testResults.xml for details! [D:\projects\runtime_copilot\src\libraries\System.Runtime\tests\System.IO.FileSystem.Tests\System.IO.FileSystem.Tests.csproj::TargetFramework=net11.0-windows]
D:\projects\runtime_copilot\eng\testing\tests.targets(210,5): error : One or more tests failed while running tests from 'System.Runtime.Tests'. [D:\projects\runtime_copilot\src\libraries\System.Runtime\tests\System.Runtime.Tests\System.Runtime.Tests.csproj::TargetFramework=net11.0-windows]
D:\projects\runtime_copilot\eng\testing\tests.targets(210,5): error : One or more tests failed while running tests from 'System.Security.Cryptography.Tests'. Please check D:\projects\runtime_copilot\artifacts\bin\System.Security.Cryptography.Tests\Release\net11.0-windows\testResults.xml for details! [D:\projects\runtime_copilot\src\libraries\System.Security.Cryptography\tests\System.Security.Cryptography.Tests.csproj::TargetFramework=net11.0-windows]
D:\projects\runtime_copilot\eng\testing\tests.targets(210,5): error : One or more tests failed while running tests from 'System.Net.Quic.Functional.Tests'. Please check D:\projects\runtime_copilot\artifacts\bin\System.Net.Quic.Functional.Tests\Release\net11.0-windows\testResults.xml for details! [D:\projects\runtime_copilot\src\libraries\System.Net.Quic\tests\FunctionalTests\System.Net.Quic.Functional.Tests.csproj::TargetFramework=net11.0-windows]
D:\projects\runtime_copilot\eng\testing\tests.targets(210,5): error : One or more tests failed while running tests from 'System.Numerics.Tensors.Tests'. Please check D:\projects\runtime_copilot\artifacts\bin\System.Numerics.Tensors.Tests\Release\net11.0\testResults.xml for details! [D:\projects\runtime_copilot\src\libraries\System.Numerics.Tensors\tests\System.Numerics.Tensors.Tests.csproj::TargetFramework=net11.0]
D:\projects\runtime_copilot\eng\testing\tests.targets(210,5): error : One or more tests failed while running tests from 'System.Numerics.Vectors.Tests'. Please check D:\projects\runtime_copilot\artifacts\bin\System.Numerics.Vectors.Tests\Release\net11.0\testResults.xml for details! [D:\projects\runtime_copilot\src\libraries\System.Numerics.Vectors\tests\System.Numerics.Vectors.Tests.csproj]
    0 Warning(s)
    8 Error(s)

Time Elapsed 00:11:37.46
Build failed with exit code 1. Check errors above.
Some builds failed:
        Configuration: release, Architecture: x64

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@adamsitnik

Copy link
Copy Markdown
Member

/ba-g the bad exit for linux test run of "System.Security.Cryptography.Tests is unrelated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.IO tenet-performance Performance related issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Don't use callbacks when performing sync IO with async handles

4 participants